Repository navigation
Attribute shaded Maven classes to their dependency packages - #2249
chinyeungli wants to merge 4 commits into
Conversation
* Added code to handle shade relocated dependency classes * Match each unmatched .class against the Maven dependency packages using the groupId, artifactId, and the class's package path. Signed-off-by: Chin Yeung Li <tli@nexb.com>
tdruez
left a comment
tdruez
left a comment
There was a problem hiding this comment.
@chinyeungli The relocation parsing makes sense, but the scoring runs on every unmapped .class of the to/ side, not only on the shaded ones. Unmapped classes of the main project get attributed to any dependency from the same group. For example, org.apache.hadoop.mapreduce.Job matches pkg:maven/org.apache.hadoop/hadoop-common with a score of 5 and is flagged as shaded-class, which is worse than leaving it as requires-review.
- What about limiting the matching to the classes under a
shadedPatternwhen relocations are available, and excluding the main package namespace otherwise? - In which cases are the dependency
DiscoveredPackagepresent in the project? Dependencies declared in the POM are created asDiscoveredDependency. The tests create those packages directly, so I'm not sure this path runs on a real shaded JAR. - How were the weights and
MIN_MATCH_SCORE = 5chosen? Were they tested on shaded JARs other than htrace-core? - The new step is never executed by the test suite:
test_scanpipe_scan_maven_package_single_fileruns without D2D, and the step only runs with D2D enabled. We still need a pipeline test with D2D, ideally on a real shaded JAR, so the relocation reversal is covered too (test_scanpipe_maven_map_shaded_classes_attributionscreates no main POM). - Missing tests for: the POM parsing failure in
get_maven_shade_relocations, the "no POM found" case inget_main_maven_pom, and the early return without dependency packages inmap_shaded_classes_to_maven_packages.
See my inline comments for the rest.
* Better code structures, tests, docstrings etc. Signed-off-by: Chin Yeung Li <tli@nexb.com>
It is now handled in
Right. I updated the code and tests.
It's chosen by the following minimum scenario:
I also checked pkg:maven/org.apache.activemq/artemis-commons@2.31.1 and the score 5 seems to work. https://repo1.maven.org/maven2/org/apache/activemq/artemis-commons/2.31.1/artemis-commons-2.31.1.pom Created #2251 for follow up.
Added test_scanpipe_scan_maven_package_d2d_shaded_classes, and it uses the real shaded-pom.xml
I think I have these covered. |
tdruez
left a comment
tdruez
left a comment
There was a problem hiding this comment.
@chinyeungli Most of the previous points are addressed. The remaining concern is still wrong attributions, see my comments on the scoring, the version deduplication and the no-relocation path.
- The new pipeline test depends on internet access: it downloads htrace-core from Maven Central and runs the full D2D. I'd mock
fetch_inputsand use a small shaded JAR fixture. - When the match is an unresolved
DiscoveredDependency, the class gets theshaded-classstatus but is not assigned to any package. The attribution only lives inextra_data, so it won't show in the package resources or in the SBOM outputs. Is this the intended result? Should aDiscoveredPackagebe created for the dependency in that case? - Missing tests:
get_maven_shade_relocationswithout a main POM (pom_resourceisNone), andmatch_shaded_class_to_packagewith a candidate that has both signals but a score belowMIN_MATCH_SCORE.
| pom_location = pom_resource.location | ||
| if not pom_location: | ||
| return relocations | ||
|
|
||
| try: | ||
| tree = SafeElementTree.parse(pom_location) | ||
| except (SafeElementTree.ParseError, OSError, ValueError) as exception: | ||
| project.add_error( | ||
| description=f"Cannot parse POM at {pom_location}: {exception}", |
There was a problem hiding this comment.
location is always a string, so the if can't be reached. Also, pom_location is the absolute path on the server. pom_resource.path is more useful in the error description.
| pom_location = pom_resource.location | |
| if not pom_location: | |
| return relocations | |
| try: | |
| tree = SafeElementTree.parse(pom_location) | |
| except (SafeElementTree.ParseError, OSError, ValueError) as exception: | |
| project.add_error( | |
| description=f"Cannot parse POM at {pom_location}: {exception}", | |
| try: | |
| tree = SafeElementTree.parse(pom_resource.location) | |
| except (SafeElementTree.ParseError, OSError, ValueError) as exception: | |
| project.add_error( | |
| description=f"Cannot parse POM at {pom_resource.path}: {exception}", |
* Use mock data for pipeline test * Created DiscoveredPackage for unresolved DiscoveredDependency * Add tests * Increase MIN_MATCH_SCORE to 6 * etc... Signed-off-by: Chin Yeung Li <tli@nexb.com>
Signed-off-by: Chin Yeung Li <tli@nexb.com>
This branch has not been deployed
2 participants
Issues
Changes
Maven Shade relocates dependency classes under a new package prefix, so they have no matching source in the development codebase. These classes were previously flagged as missing sources, which is misleading because they come from a dependency.
This PR parses the main POM for maven-shade-plugin entries and reverses them to recover the original class names. Then, it matches each unmatched
.classagainst the Maven dependency packages using thegroupId,artifactId, and the class's package path (with a scoring system). If the match meets the required score, it will then assign the matched resource's status as "shaded-class".Notes
Classes that cannot be matched unambiguously are intentionally left as
requires-reviewrather than guessed.Checklist